-
Notifications
You must be signed in to change notification settings - Fork 49
Smaller local/peers queries #528
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
17c27f3 to
dfaaf5e
Compare
…specific columns Specifically, supported_features took quite some space. Fetching only what is useful for the client. In follow-up PRs, will remove some (now) dead code that may have relied on some of those columns. Refs: scylladb/scylla-drivers#11 Signed-off-by: Yaniv Kaul <[email protected]>
dfaaf5e to
92de3f3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR optimizes database queries by replacing wildcard selects with explicit column selections for system tables. The changes reduce data transfer by only selecting the columns that are actually needed by the application.
- Replace
SELECT *with explicit column lists for system.peers and system.local queries - Maintain consistency with existing no-tokens query patterns that already use explicit column selection
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
@scylladb/python-driver-maint - if that's an acceptable path, I'll now proceed to remove code that assumed there might be more fields returning from those queries (especially DSE related stuff). |
Ah - I cannot remove those additional fields, as long as we do If we do keep this, I believe this PR is ready as is. |
|
I am thinking that to make it future proof it would be better to have a logic that reads schema of the table and returns rows that exist there from which we can combine query that will contain all the columns we want. @Lorak-mmk , WDYT ? |
How many times those tables change? It's not a bad idea to get into CI the current schema and every version compare to what we stored, but I would not add it to the driver. If we add something to core, for the driver sake, you'd assume there will be an issue for the driver side too. In any case, orthogonal to this PR. |
I don't think it is orthogonal, |
OK, I don't know how to move forward here. It wasn't broken for years, the were minimal, if any, changes, none of them were critical to this part of the code (dse_workload? dse_workloads? What else was material that was added?). I thought such a benign change could get in easily. Let me know what the next step should be, or I'll just close the PR - both options are fine by me. |
|
Imo we should ditch the DSE-related stuff, and SELECT * - let's select only what we need. |
This is the path I was going to pursue - remove some more dse stuff that now clearly is not fetched from system.local or system.peers. And removing the query to system.peers_v2 is also an option, but probably not in this PR. |
Is Scylla never going to support it? |
Never say never. But even in that case, we wouldn't like to:
But again, this is not related to this PR. I'm not going to remove what peers_v2 might have, that is material (for example, some rpc port numbers, etc.) |
|
@scylladb/python-driver-maint - do let me know how to proceed here (or drop the patch - either works). I have minor follow-ups to remove some useless .get() calls which do not fetch anything, but this is really minor. |
History wise it would be better to drop DSE part of the code and then merge this one. |
Should I then start with dropping PEERS_V2 stuff, then on top of it this item? |
|
Can we avoid dropping PEERS_V2? It is not DSE-specific. |
|
TBH, 100% proper way would be to evacuate all these queries to some class with API get all the peers. |
Is it worth the effort, vs, this small contained patch? It is modeled after previous changes done (not fetch the tokens) nearby, and it's a very simple localized change. |
Ok, I have checked all the execution paths where it is used, this PR is good. |
Pre-review checklist
./docs/source/.Fixes:annotations to PR description.